-
Notifications
You must be signed in to change notification settings - Fork 78
Kid metric added #435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kid metric added #435
Conversation
begumcig
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for implementing KID, Minette, it already looks amazing! 💜 I just left one small comment: I think we could handle the special case in compute() a bit more cleanly (maybe by adding a kid_compute to the TorchMetrics enum?), so that the compute() function stays as generic as possible. Other than that, it’s already looking great and pretty much ready to go! 💜💜
| result = self.metric.compute() | ||
|
|
||
| # Handle KID which returns a tuple (mean, std) | ||
| if self.metric_name == "kid" and isinstance(result, tuple) and len(result) == 2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have a suggestion for this yet but I think we should not do conditional statements in the compute function based on the metric name, as this can get messy quite easily, how do you feel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point, and I agree. Having conditionals in compute() based on the metric name can get messy fast. So I cleaned it up a bit:
- Took out the if
self.metric_name == "kid"check fromcompute(). - Added a
kid_computefunction to handle KID’s tuple return. - Updated the TorchMetrics enum so KID has a 4th element for
kid_compute(others stay as 3). - Now
compute()just usesself.compute_fnfrom the enum if it’s there, otherwise falls back to the defaultself.metric.compute().
So now the compute() method is generic and clean, no metric-specific conditionals. Each metric can have its own compute logic in the enum, just like how we handle the update functions.
What do you think, does this look like a good direction?
sharpenb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Left a single comment
|
This PR has been inactive for 10 days and is now marked as stale. |
begumcig
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks super good Minette! I made a small comment about the blank line but feel free to merge! Amazing job you are a queen 👸
|
This PR has been inactive for 10 days and is now marked as stale. |
2620cea to
2acf02e
Compare
…oc>=1.6.0` and `ty==0.0.1a21` for compatibility. (#417) * pin torchao version. * regenerate uv lock file as well. * fix ty dep indent * update dep for sphinx as well. * fix syntax error. * update the lock file as well. * rm linting rule. * revert. * update transformer version for janus, try not to break ci. * update the lock file as well. * pin ty version for linting. * break deps for logs. * add comment for torchao dep. * add new lock file. * pin ty version as in lock file. * pin numpydoc for jinja version conflict. * unpin transformers version. * update lockfile as well. * Kid metric added (#435) * feat/kid-metric and updated docs * fixed typo * fixed linting error * moving KID logic to enum * fixing linting error * fixed another linting error * fixing linting error * delete uv lock as per request. * add back the uv lock file. * add back the uv lock file. * regenerate uv lock file as well. * update the lock file as well. * update the lock file as well. * pin ty version for linting. * add new lock file. * pin numpydoc for jinja version conflict. * update lockfile as well. * update lock file for last time. --------- Co-authored-by: Minette Kaunismäki <[email protected]>
* feat/kid-metric and updated docs * fixed typo * fixed linting error * moving KID logic to enum * fixing linting error * fixed another linting error * fixing linting error
…oc>=1.6.0` and `ty==0.0.1a21` for compatibility. (#417) * pin torchao version. * regenerate uv lock file as well. * fix ty dep indent * update dep for sphinx as well. * fix syntax error. * update the lock file as well. * rm linting rule. * revert. * update transformer version for janus, try not to break ci. * update the lock file as well. * pin ty version for linting. * break deps for logs. * add comment for torchao dep. * add new lock file. * pin ty version as in lock file. * pin numpydoc for jinja version conflict. * unpin transformers version. * update lockfile as well. * Kid metric added (#435) * feat/kid-metric and updated docs * fixed typo * fixed linting error * moving KID logic to enum * fixing linting error * fixed another linting error * fixing linting error * delete uv lock as per request. * add back the uv lock file. * add back the uv lock file. * regenerate uv lock file as well. * update the lock file as well. * update the lock file as well. * pin ty version for linting. * add new lock file. * pin numpydoc for jinja version conflict. * update lockfile as well. * update lock file for last time. --------- Co-authored-by: Minette Kaunismäki <[email protected]>
* feat/kid-metric and updated docs * fixed typo * fixed linting error * moving KID logic to enum * fixing linting error * fixed another linting error * fixing linting error
…oc>=1.6.0` and `ty==0.0.1a21` for compatibility. (#417) * pin torchao version. * regenerate uv lock file as well. * fix ty dep indent * update dep for sphinx as well. * fix syntax error. * update the lock file as well. * rm linting rule. * revert. * update transformer version for janus, try not to break ci. * update the lock file as well. * pin ty version for linting. * break deps for logs. * add comment for torchao dep. * add new lock file. * pin ty version as in lock file. * pin numpydoc for jinja version conflict. * unpin transformers version. * update lockfile as well. * Kid metric added (#435) * feat/kid-metric and updated docs * fixed typo * fixed linting error * moving KID logic to enum * fixing linting error * fixed another linting error * fixing linting error * delete uv lock as per request. * add back the uv lock file. * add back the uv lock file. * regenerate uv lock file as well. * update the lock file as well. * update the lock file as well. * pin ty version for linting. * add new lock file. * pin numpydoc for jinja version conflict. * update lockfile as well. * update lock file for last time. --------- Co-authored-by: Minette Kaunismäki <[email protected]>
* feat/kid-metric and updated docs * fixed typo * fixed linting error * moving KID logic to enum * fixing linting error * fixed another linting error * fixing linting error
…oc>=1.6.0` and `ty==0.0.1a21` for compatibility. (#417) * pin torchao version. * regenerate uv lock file as well. * fix ty dep indent * update dep for sphinx as well. * fix syntax error. * update the lock file as well. * rm linting rule. * revert. * update transformer version for janus, try not to break ci. * update the lock file as well. * pin ty version for linting. * break deps for logs. * add comment for torchao dep. * add new lock file. * pin ty version as in lock file. * pin numpydoc for jinja version conflict. * unpin transformers version. * update lockfile as well. * Kid metric added (#435) * feat/kid-metric and updated docs * fixed typo * fixed linting error * moving KID logic to enum * fixing linting error * fixed another linting error * fixing linting error * delete uv lock as per request. * add back the uv lock file. * add back the uv lock file. * regenerate uv lock file as well. * update the lock file as well. * update the lock file as well. * pin ty version for linting. * add new lock file. * pin numpydoc for jinja version conflict. * update lockfile as well. * update lock file for last time. --------- Co-authored-by: Minette Kaunismäki <[email protected]>
Description
Added KID (Kernel Inception Distance) metric
Related Issue
Fixes #(issue number)
Type of Change
How Has This Been Tested?
The KID metric implementation has been tested with the following test suite:
Main KID Test (
test_kid):subset_sizeparameter (must be smaller than number of samples)test_kid[LAION256-cpu]andtest_kid[LAION256-cuda]- both PASSEDCall Type Tests (
test_check_call_type):gt_yfor single,pairwise_gt_yfor pairwise)test_check_call_type[single-kid]andtest_check_call_type[pairwise-kid]- both PASSEDAll Torch Metrics Tests:
uv run pytest tests/evaluation/test_torch_metrics.py -vChecklist
Additional Notes